Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harden passing authentication type through hydra #20

Merged
merged 10 commits into from
Jul 7, 2023

Conversation

chaireze
Copy link
Collaborator

@chaireze chaireze commented Jun 27, 2023

When I wrote the code to include workload identity authentication I left some edge cases unaccounted for

The code here does the following:

  • a fix on the patch that checks workload identity variables while grabbing the cloud config from a secret
  • lets us pass an 'Authentication' parameter in our storage classes to dictate how we want to authenticate with our storage blob (a bit more details here)
  • adds logic to be selective about what annotations are added to the pvc depending on the type of authentication requested
  • tries to add some hardening for when the volume attempts to be provisioned, sometimes the same pv/pvc pair runs through our block in 'nodeserver.go'

Here is a PR in hydra that uses these changes: https://msazure.visualstudio.com/One/_git/Hydra/pullrequest/8348654

@coveralls
Copy link

coveralls commented Jun 29, 2023

Pull Request Test Coverage Report for Build 5469141894

  • 84 of 118 (71.19%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 78.888%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/blob/controllerserver.go 0 2 0.0%
pkg/edgecache/cachevolume/pvc_annotator.go 84 88 95.45%
pkg/util/pvutil.go 0 7 0.0%
pkg/blob/nodeserver.go 0 10 0.0%
pkg/blob/azure.go 0 11 0.0%
Totals Coverage Status
Change from base Build 5356313631: 0.9%
Covered Lines: 2085
Relevant Lines: 2643

💛 - Coveralls

@chaireze chaireze marked this pull request as ready for review June 29, 2023 14:56
pleaseDefine
pleaseDefine previously approved these changes Jul 3, 2023
Copy link
Collaborator

@pleaseDefine pleaseDefine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard, maybe PVCAnnotator instead of CVHelper?
generally lgtm!

avoltz
avoltz previously approved these changes Jul 5, 2023
@chaireze chaireze merged commit 441e632 into staging Jul 7, 2023
13 checks passed
@chaireze chaireze deleted the echairez/figure-out-better-auth-check branch July 7, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants